Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[exporter/prometheusremotewrite] prometheusremotewrite exporter add option to send metadata #27565

Conversation

jmichalek132
Copy link
Contributor

@jmichalek132 jmichalek132 commented Oct 9, 2023

Description:

This PR adds an option to send metric Metadata to prometheus compatible backend (disabled by default). This contains information such as metric descrtiption, type, unit, and name.

Link to tracking Issue: #13849

Testing:

Tested in our testing environment with locally built image.

Documentation:

@jmichalek132
Copy link
Contributor Author

Hi, @Aneurysm9 @rapphil as I said here
#13849 (comment)

So after chatting with @gouthamve on Promcon, he suggested I should just choose a simple option for implementing support for sending the metadata in the remote write exporter and put it behind a config option. This is what I came up with. I would like some feedback on this before I add tests. Basically the same way we batch the metrics into a couple of requests that are below the max size of a single one, we repeat the same for metadata and send it as a separate set of requests.

@bryan-aguilar
Copy link
Contributor

The one thing that stands out is that metadata is listed as Future Plans on the remote write spec.

We talked about this in the Collector Sig on 10/18 and @jmichalek132 provided some clarification that upstream Prometheus already sends metadata but just less frequently. Current proposed implementation here would send it more often so that we don't have to maintain a metric -> metadata map.

Also, implementation is opt in and behind feature gate here.

Also, PRW 1.1 spec is in flight which contains metadata related changes. I have not read it yet and thus cannot provide a summary.

Please provide corrections for anything I may have missed here.

@jmichalek132
Copy link
Contributor Author

The one thing that stands out is that metadata is listed as Future Plans on the remote write spec.

We talked about this in the Collector Sig on 10/18 and @jmichalek132 provided some clarification that upstream Prometheus already sends metadata but just less frequently. Current proposed implementation here would send it more often so that we don't have to maintain a metric -> metadata map.

Also, implementation is opt in and behind feature gate here.

Also, PRW 1.1 spec is in flight which contains metadata related changes. I have not read it yet and thus cannot provide a summary.

Please provide corrections for anything I may have missed here.

  • Metadata is currently send by default already in prometheus when using remote write (config).
  • For the PRW 1.1. spec, the aim is to basically do the same thing as OTLP already does, which is sending the metadata in every request with the metric, I would be also willing to implement these changes in remote write exporter.

@jmichalek132 jmichalek132 force-pushed the jm-feat-prometheus-remote-write-send-metadata-2 branch from 84e7e31 to 52e6c95 Compare October 28, 2023 18:28
@jmichalek132 jmichalek132 force-pushed the jm-feat-prometheus-remote-write-send-metadata-2 branch from 44dcea6 to 44c22dd Compare October 29, 2023 10:16
@jmichalek132 jmichalek132 changed the title Prometheus remote write exporter send metadata [exporter/prometheusremotewrite] prometheusremotewrite exporter add option to send metadata Nov 9, 2023
@jmichalek132 jmichalek132 marked this pull request as ready for review November 9, 2023 14:55
@jmichalek132 jmichalek132 requested a review from a team November 9, 2023 14:55
@jmichalek132
Copy link
Contributor Author

jmichalek132 commented Nov 11, 2023

Pipeline failiures are unrelated to my changes, and come from other exporters.

@jmichalek132
Copy link
Contributor Author

jmichalek132 commented Nov 21, 2023

Re-tested in our dev env, to make sure everything still works (sending data to mimir).

Metadata nicely showing up in Grafana.

Screenshot 2023-11-21 at 12 34 56

We do see a roughly doubling of the requests sent (which makes sense, for every timeseries request we send and extra request carrying metadata request. But it is disabled by default. Also in the coming remote write 1.1 metadata will be send along side timeseries data in the same request anyway (this is also done in OTLP).
Screenshot 2023-11-21 at 12 33 06

No errors in logs of either mimir / otel collector or in metrics.

@jmichalek132
Copy link
Contributor Author

Hi, @rapphil @dashpole could you please review this again when you have a chance so we can get this merged :) ?

@dashpole dashpole added the ready to merge Code review completed; ready to merge by maintainers label Nov 22, 2023
@jpkrohling jpkrohling merged commit a0e6491 into open-telemetry:main Nov 22, 2023
88 checks passed
@github-actions github-actions bot added this to the next release milestone Nov 22, 2023
RoryCrispin pushed a commit to ClickHouse/opentelemetry-collector-contrib that referenced this pull request Nov 24, 2023
…ption to send metadata (open-telemetry#27565)

**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->

This PR adds an option to send metric Metadata to prometheus compatible
backend (disabled by default). This contains information such as metric
descrtiption, type, unit, and name.

**Link to tracking Issue:** <Issue number if applicable> open-telemetry#13849

**Testing:** <Describe what testing was performed and which tests were
added.>

Tested in our testing environment with locally built image.

**Documentation:** <Describe the documentation added.>

---------

Co-authored-by: Antoine Toulme <[email protected]>
Co-authored-by: Anthony Mirabella <[email protected]>
@jmichalek132 jmichalek132 deleted the jm-feat-prometheus-remote-write-send-metadata-2 branch January 14, 2024 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exporter/prometheusremotewrite pkg/translator/prometheus ready to merge Code review completed; ready to merge by maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants